Skip to content

fix(rees): stop dropping added ++-prefixed lines in the duplication scan#2571

Closed
ultrahighsuper wants to merge 1 commit into
JSONbored:mainfrom
ultrahighsuper:fix/rees-duplication-scan-plus-prefix
Closed

fix(rees): stop dropping added ++-prefixed lines in the duplication scan#2571
ultrahighsuper wants to merge 1 commit into
JSONbored:mainfrom
ultrahighsuper:fix/rees-duplication-scan-plus-prefix

Conversation

@ultrahighsuper

Copy link
Copy Markdown
Contributor

Summary

extractAddedBlocks in review-enrichment/src/analyzers/duplication-scan.ts parses a unified diff into blocks of added significant lines with their new-file line numbers (used to locate copy-paste duplication). Inside the hunk loop it had:

if (!inHunk) continue;
if (line.startsWith("+++")) continue; // file header inside the patch, not an added line
if (line.startsWith("+")) {  newLine++; }

The +++ skip is meant to drop the +++ b/file diff header — but that header only appears in the patch preamble, before the first @@, which the if (!inHunk) continue; guard right above already drops. So inside a hunk, a line starting with +++ is never a header: it is an added line whose content begins with ++ (e.g. a pre-increment statement ++counterValueForLoop; → patch line +++counterValueForLoop;).

The stray skip therefore (a) drops that real added line from the block, and (b) since it continues before newLine++, shifts every following added line's number down by one. Downstream, scanDuplicationlongestSharedRun reports DuplicationFinding.line / sourceLine off by one for any run following such a line, and a copy-pasted run that starts with a ++-line can be missed.

Fix: remove the redundant +++ skip so +++… lines fall through to the normal + handling (normalized and counted). Headers remain handled by the !inHunk guard.

Note: this is the only analyzer where the +++ skip is redundant — the others (secret-scan, redos, etc.) don't track inHunk and use the marker to skip preamble headers, so they have a different structure and are out of scope here.

No linked issue: small, self-contained diff-parsing fix in one helper; per this repo's linkedIssuePolicy: preferred, a direct PR with this rationale is appropriate.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm --prefix review-enrichment run build (tsc clean)
  • npm --prefix review-enrichment run test — the duplication-scan suite passes (36 tests) with a new regression test asserting an added ++counterValueForLoop; line is kept and the block's line numbers stay [10, 11, 12] (not [10, 11]). The test was confirmed to FAIL against the unpatched code (the line was dropped and numbers shifted).
  • New/changed behavior has a regression test

If any required check was skipped, explain why:

  • The change is one branch in a pure helper in review-enrichment/src; it adds/changes no analyzer, so analyzer-metadata.json is unaffected. Codecov's src/** patch gate does not apply to review-enrichment/**; this package's own node --test suite covers it.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no such changes.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (Internal diff parsing only; no schema change.)
  • No visible UI change in this PR.

Notes

  • Reproduction: extractAddedBlocks("@@ -1,0 +10,3 @@\n+++counterValueForLoop;\n+const secondSignificantLineHere = makeValue(2)\n+const thirdSignificantLineHere = makeValue(3)") previously returned one block with lineNos [10, 11] (the ++ line dropped, following lines shifted); it now returns lineNos [10, 11, 12] with all three lines.

… scan

extractAddedBlocks skipped any in-hunk line starting with `+++` as a "file
header", but the `+++ b/file` header only appears in the patch preamble, which
the `!inHunk` guard already drops. Inside a hunk, `+++…` is an added line whose
content begins with `++` (e.g. a pre-increment `++x;`): the skip dropped that
real line from the block AND shifted every following added line's number down by
one (it never advanced the new-file counter). Remove the redundant skip so such
lines are normalized and numbered like any other added line.
@ultrahighsuper
ultrahighsuper requested a review from JSONbored as a code owner July 2, 2026 09:11
@dosubot dosubot Bot added the size:XS label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 09:10:28 UTC

2 files · 1 AI reviewer · no blockers · readiness 75/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
The diff correctly fixes `extractAddedBlocks` so an added hunk line whose content begins with `++` is normalized and counted instead of being mistaken for a `+++ b/file` header. Because `inHunk` is already false for the patch preamble, removing the inner `+++` skip preserves header handling while restoring both the duplicated-line content and following new-file line numbers. The added regression covers the reachable failure mode for a hunk-local `+++...` added line.

Nits — 4 non-blocking
  • nit: `review-enrichment/test/duplication-scan.test.ts` should include a real `--- a/file` / `+++ b/file` preamble in this regression test so the test proves both halves of the claim: preamble headers are still ignored and hunk-local `+++...` lines are kept.
  • nit: `review-enrichment/src/analyzers/duplication-scan.ts` now carries a long explanatory comment for a small parser rule; consider tightening it after the regression test captures the edge case.
  • In `review-enrichment/test/duplication-scan.test.ts`, prepend `--- a/example.ts` and `+++ b/example.ts` before the `@​@​` line in the new patch fixture and assert the block still starts at line 10.
  • In `review-enrichment/src/analyzers/duplication-scan.ts`, shorten the new comment to the invariant that matters: preamble headers are skipped before `inHunk`, so hunk-local `+++` is real added content.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 30 registered-repo PR(s), 20 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor ultrahighsuper; Gittensor profile; 30 PR(s), 0 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: ultrahighsuper
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 30 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflicts, a previous PR already handled this.

This branch has conflicts that must be resolved
Use the [web editor](https://github.com/JSONbored/gittensory/pull/2571/conflicts) or the command line to resolve conflicts before continuing.

review-enrichment/src/analyzers/duplication-scan.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

2 participants